167 combinatorial test bed fixes#169
Conversation
|
Warning Review limit reached
More reviews will be available in 59 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors the combinatorial page module into a modular factory-based architecture. It adds estimation calculations for cartesian row counts and theoretical minimum coverage rows, displays live estimates in a new UI container, implements an async confirmation flow for cartesian-product generation above a 10,000-row threshold, provides progress indicators with running state animation, and includes comprehensive Jest tests covering estimation logic, confirmation flow, and page initialization. ChangesCombinatorial Estimation & Progress Workflow
Sequence Diagram(s)sequenceDiagram
participant User
participant initCombinatorialPage
participant createCombinatorialPage
participant DOM
participant Algorithms
participant Results
User->>initCombinatorialPage: DOM ready
initCombinatorialPage->>DOM: Find elements
initCombinatorialPage->>createCombinatorialPage: Construct with DOM refs
createCombinatorialPage->>DOM: Render strength/strategy controls
createCombinatorialPage->>DOM: Update estimates on strength change
User->>Results: Click generate
Results->>createCombinatorialPage: Validate & filter algorithms
createCombinatorialPage->>Algorithms: Run sequentially with yields
Algorithms->>DOM: Update progress/status
Algorithms->>Results: Capture output & failures
createCombinatorialPage->>Results: Sort by row count & render table
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #167 by improving the Combinatorial Strategies page UX: it adds up-front size/coverage estimates, adds a running/progress indicator while strategies execute, removes cartesian product from default selection, and sorts strategy metrics by smallest row count first.
Changes:
- Show cartesian size + n-wise lower-bound/tuple estimates and update them when the selected n changes.
- Add a running/progress indicator and update status as the generator switches strategies.
- Make results ordering deterministic by sorting strategy metrics (and details) by fewest rows first, and gate large cartesian runs behind a confirmation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/web/styles.css | Adds layout fixes plus new styles for estimates panel and running/progress spinner. |
| apps/web/combinatorial.html | Adds the estimates container and a dedicated progress/status element for live updates. |
| apps/web/src/combinatorial-entry.js | Implements estimates calculation, progress updates, cartesian confirmation filtering, and sorted results rendering. |
| apps/web/src/tests/jest/combinatorial-entry.test.js | Adds Jest coverage for estimates, cartesian confirmation filtering, and default-selection/estimate updates. |
Greptile SummaryThis PR migrates the combinatorial page entry point from
Confidence Score: 5/5Safe to merge — the core generation path is correct, the button re-enable regression is fixed with try/finally, and the new estimate/progress features are additive. The refactored module correctly guards all the edge cases that were broken in the old .js file. The generate button is now always re-enabled via finally, formatNumber uses an explicit locale, and no new data-loss or breakage paths were introduced. apps/web/src/combinatorial-entry.mjs — the two standalone calculation helpers create redundant model instances; minor cleanup suggested. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User clicks Generate] --> B[parseEnumParameters]
B --> C{Errors or < 2 params?}
C -- yes --> D[setStatus error, return]
C -- no --> E[renderControls]
E --> F[getSelectedAlgorithms]
F --> G{0 algorithms?}
G -- yes --> H[setStatus error, return]
G -- no --> I[filterAlgorithmsForCartesianConfirmation]
I --> J{Cartesian + row count > threshold?}
J -- no --> K[syncSelectedAlgorithms]
J -- yes --> L[Show confirm dialog]
L -- cancel --> M[Remove cartesian from list]
L -- ok --> K
M --> N{0 algorithms remain?}
N -- yes --> O[setStatus warning, return]
N -- no --> K
K --> P[generateButton.disabled = true]
P --> Q[Loop over algorithms]
Q --> R[setProgress running]
R --> S[yieldToBrowser]
S --> T[runAlgorithm]
T --> U{Throws?}
U -- yes --> V[push error result]
U -- no --> W[push success result]
V --> X[renderSummary + renderDetails]
W --> X
X --> Y[yieldToBrowser]
Y --> Q
Q -- done --> Z[setStatus completed]
Z --> AA[finally: button.disabled=false, setProgress empty]
Reviews (5): Last reviewed commit: "Scope combinatorial entry ESM fix" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/styles.css (1)
1260-1271: ⚡ Quick winRespect reduced-motion preferences for spinner animation
The running spinner animates indefinitely. Add a reduced-motion fallback to avoid unnecessary motion for users who opt out.
Suggested refactor
`@keyframes` combinatorial-spin { to { transform: rotate(360deg); } } + +@media (prefers-reduced-motion: reduce) { + .combinatorial-progress[data-state='running']::before { + animation: none; + } +}Also applies to: 1338-1342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/styles.css` around lines 1260 - 1271, The spinner animation defined on the .combinatorial-progress[data-state='running']::before selector currently animates indefinitely; add a reduced-motion fallback by using the prefers-reduced-motion media query to disable the combinatorial-spin animation (and reset any transforms) when users opt out — e.g., add `@media` (prefers-reduced-motion: reduce) { .combinatorial-progress[data-state='running']::before { animation: none; /* reset transforms if needed */ } } and apply the same change for the other spinner block that uses the combinatorial-spin keyframes to ensure both locations respect reduced-motion preferences.apps/web/src/tests/jest/combinatorial-entry.test.js (1)
64-69: ⚡ Quick winMake the confirmation assertion less copy-fragile.
This test currently hard-codes the full dialog message text, which will fail on minor wording edits while behavior is still correct. Assert stable contract fields plus the row-count content instead.
Suggested diff
- expect(requestConfirm).toHaveBeenCalledWith({ - title: 'Cartesian product generation', - message: 'You included cartesian product generation. Are you sure? this will generate 100,000 data rows.', - okLabel: 'Run cartesian product', - cancelLabel: 'Skip cartesian product', - }); + expect(requestConfirm).toHaveBeenCalledWith( + expect.objectContaining({ + title: 'Cartesian product generation', + okLabel: 'Run cartesian product', + cancelLabel: 'Skip cartesian product', + message: expect.stringContaining('100,000'), + }) + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/tests/jest/combinatorial-entry.test.js` around lines 64 - 69, The assertion for requestConfirm is too brittle; update the test that calls requestConfirm to assert the stable fields (title equals 'Cartesian product generation', okLabel and cancelLabel text) and that the message contains the dynamic row-count number (e.g., /100,000/ or computed rows variable) rather than exact full message text. Locate the expectation using requestConfirm in combinatorial-entry.test.js and replace the toHaveBeenCalledWith full-string check with checks for the specific properties and a partial-match (string contains / regex) on the message field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/styles.css`:
- Around line 1253-1271: The .combinatorial-progress text color and spinner
(rules .combinatorial-progress and
.combinatorial-progress[data-state='running']::before) use `#1f5f80` which is
low-contrast on dark backgrounds; update the CSS to use theme-aware colors
(e.g., CSS variables or a prefers-color-scheme: dark override) so both the text
color and the spinner border-top-color change in dark mode to a lighter/contrast
color; modify the rules for .combinatorial-progress and
.combinatorial-progress[data-state='running']::before to reference the theme
variables (or add a dark media query) rather than the hard-coded `#1f5f80` so the
progress indicator remains readable in dark theme.
---
Nitpick comments:
In `@apps/web/src/tests/jest/combinatorial-entry.test.js`:
- Around line 64-69: The assertion for requestConfirm is too brittle; update the
test that calls requestConfirm to assert the stable fields (title equals
'Cartesian product generation', okLabel and cancelLabel text) and that the
message contains the dynamic row-count number (e.g., /100,000/ or computed rows
variable) rather than exact full message text. Locate the expectation using
requestConfirm in combinatorial-entry.test.js and replace the
toHaveBeenCalledWith full-string check with checks for the specific properties
and a partial-match (string contains / regex) on the message field.
In `@apps/web/styles.css`:
- Around line 1260-1271: The spinner animation defined on the
.combinatorial-progress[data-state='running']::before selector currently
animates indefinitely; add a reduced-motion fallback by using the
prefers-reduced-motion media query to disable the combinatorial-spin animation
(and reset any transforms) when users opt out — e.g., add `@media`
(prefers-reduced-motion: reduce) {
.combinatorial-progress[data-state='running']::before { animation: none; /*
reset transforms if needed */ } } and apply the same change for the other
spinner block that uses the combinatorial-spin keyframes to ensure both
locations respect reduced-motion preferences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 48ae0eae-0dcd-4972-be76-a2cd396cedbd
📒 Files selected for processing (4)
apps/web/combinatorial.htmlapps/web/src/combinatorial-entry.jsapps/web/src/tests/jest/combinatorial-entry.test.jsapps/web/styles.css
| selectedAlgorithms = await filterAlgorithmsForCartesianConfirmation({ | ||
| algorithms: selectedAlgorithms, | ||
| parameters, | ||
| requestConfirm, | ||
| }); | ||
| syncSelectedAlgorithms(selectedAlgorithms); | ||
|
|
||
| if (selectedAlgorithms.length === 0) { | ||
| setStatus('Cartesian product run cancelled. No strategies remain selected.', 'warning'); | ||
| setProgress(''); | ||
| return; | ||
| } | ||
|
|
||
| generateButton.disabled = true; |
There was a problem hiding this comment.
generateButton.disabled = true is only set on line 494, after filterAlgorithmsForCartesianConfirmation has been awaited. When the confirm dialog is shown (cartesian product selected + row count > threshold), there is a window during which the button remains enabled. Clicking it a second time starts a second concurrent generateCombinatorial call, both of which will write to the same summaryRoot and detailsRoot DOM nodes and each will independently set the button back to enabled in their finally blocks, leaving execution state inconsistent.
Moving generateButton.disabled = true to before the filterAlgorithmsForCartesianConfirmation await (and re-enabling it in an early-return path if needed) prevents the race.
closes #167
Summary by CodeRabbit
New Features
UI/UX